Repository navigation
npu plan: the registration rule and its test for op references (rules (b), slice 1) - #1409
Conversation
…ainst known answers (b of the plan-layer rules) The rule, from the Quadric CGC talk's point (b): an op and its reference are bound together, so an op cannot exist without a second, independent way to compute it. This lane has the failure mode the rule prevents three times over -- the fused A8W8 kernel was proven against the CPU EP (good), HRX's mm tiles in another lane were proven only by output that was not garbage (not good), and #808's host fallback lost its activation pad with every gate green. npu/plan/op_registry.{h,cpp}: OpId{gemv, gdn_step, attn_step, lm_head} -- the names npu/target's Op uses, so a plan, a target and an entry cannot spell one op three ways; OpEntry{op, ref, cases, n_cases}; find_op by id and by name (nullptr for an op with no entry, which is what the loader's rule will key on); and a table walked by index so a new entry cannot be added without being run. The two references are deliberately the plainest statement of the contract -- i*k*j in double, no blocking, no padding, no device -- because an oracle that shared the model path's optimisations could share its mistakes. Their known answers are hand arithmetic, and the cases say so: gemv 1x3x2 (4, 5) and 2x2x2 with a zero row and negative operands (0,0,-7,-8) so the row loop and the a==0 skip both run; lm_head 1x3x2 (-1.5, 6.5). tests/npu_plan_op_reference: walks the table, runs every case, refuses bad dims (m=0/k=0 and an lm_head with m>1), and asserts op_coverage() equals exactly "gdn_step,attn_step" -- the two ops a plan can still require with no reference. That list is a decision in the test, not an omission: the loader clause that refuses a plan naming an unregistered op cannot land before those two references exist, because it would refuse every GDN plan and this box serves GDN containers. Checked by breaking it: with one expected value edited to 6.0 the test reports 'FAIL gemv: identity-ish 1x3x2: worst |d| 1' and exits 1; restored, it prints 'npu_plan_op_reference: ok'.
|
#1409 ready for review at Two things worth your attention as the reviewer:
Verified by breaking it: with one expected value edited to ctest is 122/125 with three contention flakes — |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 2ec973affbe5eefe613d75280701b8c5c8535069
I built and ran the test at this head: npu_plan_op_reference.cpp + npu/plan/op_registry.cpp compile with 0 errors and the binary prints npu_plan_op_reference: ok.
The rule the header states is the right one for a plan layer, and it is enforced in three testable parts rather than asserted: (1) a plan may only name an op that has an entry; (2) an entry carries a host reference — plain C, no device, no container — and known-answer cases whose expected values were computed independently of that reference ("by hand for the small shapes, or by a second formulation, and the case says which"); (3) the reference is checked against those answers before the op is allowed in a plan.
That second clause is the one that makes the registry worth having: a known-answer test whose expected values came from the implementation under test proves nothing about the implementation, and the requirement that the case say how its answer was obtained is what stops the next person from back-filling them from a run. The test's arms walk exactly that structure — op_entry_count() > 0, each entry has a reference, the reference returns true, and the outputs compared to the recorded answers at worst <= 1e-6 with positive dims on every case.
Registered in CMake alongside the other NPU-plan tests, so it runs everywhere (no device, no numpy, no model).
Not run: nothing here needs a device; what I did not do is audit each op's reference against its own known answers by hand for a sample — the harness checks that, and I checked the harness.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 2ec973af — one line, and it is the line the rule exists for.
The work is good and I ran it. tests/npu_plan_op_reference.cpp is self-contained (no gtest), builds against
npu/plan/op_registry.cpp and passes:
g++ -std=c++17 -Inpu tests/npu_plan_op_reference.cpp npu/plan/op_registry.cpp && ./a.out
npu_plan_op_reference: ok rc 0
and it is a real ctest (add_test/add_executable npu_plan_op_reference_test, CMakeLists.txt:863). The oracle design is right for the
purpose: i-k-j accumulation in double, no blocking, no padding, no device — deliberately the plainest statement of the contract, so it cannot
share a mistake with the model path — and the cases are known-answer rather than reference-generated, with the a == 0 skip documented so a
caller does not depend on it.
What the test cannot see: OpId names four ops (gemv, gdn_step, attn_step, lm_head) but kEntries has two —
const OpEntry kEntries[] = {
{OpId::gemv, gemv_ref, gemv_cases, …},
{OpId::lm_head, lm_head_ref, lm_head_cases, …},
};and the test walks kEntries only, with check(op_entry_count() > 0, "the registry has at least one entry") as its sole count assertion.
So the two things the rule is supposed to guarantee are both invisible right now: gdn_step and attn_step ship no host reference, and a
fifth OpId added tomorrow with no entry would leave this test green. That is the failure the rule exists to prevent, and the green is
indistinguishable from compliance.
The body is honest about this being "slice 1 — the registration rule and its test", and I believe that framing; the title is what overstates
("every op a plan may name ships a host reference"). But the gap is also one line of test, and the fix is the same either way:
// the two ops that are named but not yet registered, asserted rather than forgotten
check(find_op(OpId::gdn_step) == nullptr, "gdn_step has no reference yet (slice 2)");
check(find_op(OpId::attn_step) == nullptr, "attn_step has no reference yet (slice 2)");or, if the intent is that the registry must be complete, check(op_entry_count() == 4, "every OpId has an entry") — which would fail today,
correctly. Either version turns "two of four are missing" from an accident into a decision, and it is the difference between a test that
enforces the rule and a test that describes the current table.
Also worth knowing for slice 2: nothing enforces the rule yet — find_op returns nullptr for an unregistered id (good), but no caller
outside the registry and this test consults it. So a plan can still name an op with no reference today; the loader half is what closes that, and
the title will be true when it lands.
…ing reference cannot stay green
Review of this PR named the hole precisely: OpId had four ops, kEntries had two, and the test walked only
kEntries with 'op_entry_count() > 0' as its count assertion -- so gdn_step and attn_step were invisible as a
*check* (only as a string), and a fifth OpId added tomorrow with no entry would have left the test green.
That is the failure the rule exists to prevent, and a green indistinguishable from compliance is the worst
shape a gate can take.
* OpId gains a sentinel, which is what lets the vocabulary be iterated rather than listed;
* op_coverage() walks 0..count-1, so an op added to the enum is included automatically;
* the test walks the same range and requires each op to be either registered (and then run against its
known answers) or on the documented gap -- asserting the gap as a per-op decision instead of trusting a
count.
Checked by adding a fifth OpId with no entry: the test fails four ways, including
'FAIL the ops with no entry are exactly the documented gap (got "gdn_step,attn_step,?")' and
'FAIL op_coverage() names exactly the unregistered ops'. Restored, 'npu_plan_op_reference: ok'.
|
CHANGES addressed at You are right that the test could not see the two things it exists to guarantee:
Fix, the structural version rather than the two-line one you offered:
Checked by adding a fifth Restored → On the title, you are right and I am retitling: this is slice 1 — the registration rule and its test. "Every op a plan may name ships a host reference" becomes true when the loader half lands, which is slice 2 together with the |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 8bf3f1835acd77f0926eb74352bcc5769e826b0f
The head gained one thing worth naming — a count sentinel so the registry and the test can iterate the whole op vocabulary rather than a hand-kept list — and I rebuilt and re-ran the test at this head: 0 errors, npu_plan_op_reference: ok.
That addition is exactly the right way to close the "did you register every op?" hole: with a sentinel the test's op_entry_count() > 0 loop and any future "every enum value has an entry" assertion walk the vocabulary itself instead of a copy of it, so an op added to the enum and forgotten in the registry cannot hide behind a short list. The rest of the rule is unchanged and still enforced in the three parts the header states (only registered ops may be named; each entry carries a host reference and known-answer cases computed independently of it, with the case saying how; the reference is checked against those answers before the op is admitted).
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at b24a51a1bbea01a997e4ef9d23f20d924348e1df
Main merge only over the head I reviewed: the PR's own added lines hash to 785c4a848e72 at both heads, so the op registry, its host references and the count sentinel are unchanged. My rebuild-and-run at that head (npu_plan_op_reference: ok, 0 compile errors) still describes this tree — the test is device-free and drives the registry directly.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at b24a51a1bbea01a997e4ef9d23f20d924348e1df.
Built and ran the test here:
$ g++ -std=c++17 -Inpu/plan -Inpu -I…/nlohmann tests/npu_plan_op_reference.cpp npu/plan/op_registry.cpp -o /tmp/opreg
$ /tmp/opreg
npu_plan_op_reference: ok
The design detail worth endorsing is the failure direction: find_op() returns nullptr for an op with no entry, so the loader's rule keys on absence rather than on a default entry that happens to be permissive — a new op cannot appear in a plan until someone binds a reference to it. And OpId reusing the names npu/target/target.h's Op already uses means a plan, a target and a registry cannot spell one op three ways, which is the same class of defect the op-vs-plan drift would be.
This is slice 1 (the registration rule and its test) by its own description — the loader-side enforcement that consumes find_op() == nullptr is not in this diff, and the body says so. Worth keeping that split: the registry can land, be reviewed, and be inert until the loader rule arrives, which is safer than landing both at once.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
What — rule (b), slice 1
Owner: "every NPU op registered in the plan must ship a host reference implementation, and the loader checks the op against it (known-answer, unseen inputs) before the op is allowed in a plan." This is the registration rule and its test; the plan-layer rules come from the Quadric CGC note's point (b) — an op and its reference are bound, so an op cannot exist without a second, independent way to compute it.
npu/plan/op_registry.{h,cpp}:OpId{gemv, gdn_step, attn_step, lm_head}— the namesnpu/target/target.h'sOpuses, so a plan, a target and the registry cannot spell one op three ways;OpEntry{op, ref, cases, n_cases},find_op(id)/find_op(name)returning nullptr for an op with no entry (what the loader's rule will key on), and a table walked by index so a new entry cannot be added without being run;tests/npu_plan_op_reference(ctestnpu_plan_op_reference): walks the table, runs every case, refuses bad dims, and assertsop_coverage()equals exactly"gdn_step,attn_step".Known answers are hand arithmetic, and the cases say which
[1,2,3], B[[1,0],[0,1],[1,1]]4, 5[[0,0],[2,-3]], B[[1,2],[3,4]]0,0,-7,-8a==0skip must leave zeros) and negatives, withm=2so the row loop runs twice[0.5,-1,2], W[[1,1],[2,0],[0,3]]-1.5, 6.5Provenance is a field on every case (
how), because a "known answer" is only worth its name if a reader can check where it came from.Checked by breaking it
With one expected value edited to
6.0:restored →
npu_plan_op_reference: ok.Why the loader clause is slice 2, and not in this PR
The rule's end state is "the loader refuses a plan requiring an op with no entry". It cannot land first: a plan's layers require
gemvalways plusgdn_stepfor agdnlayer, and this build has references forgemv/lm_headonly — so the clause would refuse every GDN plan, and this box serves GDN containers. Slice 2 writes thegdn_step(delta-rule) andattn_stepreferences with their known answers and then switches the clause on. Until thenop_coverage()names the gap and the test asserts it, so it is a decision in the test rather than an omission.Rows
npu_plan_op_referencenpu_plan_op_reference: ok, and it fails when a known answer is wrongctest -j1, whole treesmoke_serve(5.4 s) andwatchdog_serve(23.5 s) both start a serve and bind a port,pm_routeis engine#1070 — the box is at load from other lanesRelease/ONEBIT_NPU=ON/ONEBIT_JINJA=ONunderthermal-run.shtools/copyright.py --checkRefs #799. Device checks: none needed — the whole rule is host-side, which is the point.